Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block API: Ensure backwards compatibility for block matchers #3519

Merged
merged 2 commits into from
Nov 20, 2017

Conversation

youknowriad
Copy link
Contributor

with #2854 it's no longer possible to use attribute matchers to define the block attributes. This PR adds a way to ensure backwards compatibility while warning about the "deprecated" status of these matchers.

Testing instructions

  • Update any block's attributes definition to the old matchers signature (example: source.attr( 'myattribute' ))
  • Load Gutenberg
  • the block should still work as expected
  • A warning should show up in the console when loading the editor.

@youknowriad youknowriad added the [Feature] Block API API that allows to express the block paradigm. label Nov 16, 2017
@youknowriad youknowriad self-assigned this Nov 16, 2017
@youknowriad youknowriad requested review from mtias and aduth November 16, 2017 13:53
@codecov
Copy link

codecov bot commented Nov 16, 2017

Codecov Report

Merging #3519 into master will increase coverage by 0.05%.
The diff coverage is 46.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3519      +/-   ##
=========================================
+ Coverage   35.04%   35.1%   +0.05%     
=========================================
  Files         263     265       +2     
  Lines        6705    6735      +30     
  Branches     1220    1221       +1     
=========================================
+ Hits         2350    2364      +14     
- Misses       3677    3693      +16     
  Partials      678     678
Impacted Files Coverage Δ
blocks/library/button/index.js 9.3% <ø> (ø) ⬆️
blocks/api/matchers.js 92.85% <ø> (ø)
blocks/api/parser.js 98% <ø> (ø) ⬆️
blocks/hooks/index.js 100% <100%> (ø) ⬆️
blocks/index.js 100% <100%> (ø)
blocks/hooks/matchers.js 42.85% <42.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d79f94...d8249a9. Read the comment docs.

function warnAboutDeprecatedMatcher() {
// eslint-disable-next-line no-console
console.warn(
'Attributes matchers are deprecated and they will be remove in future version of Gutenberg.' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "remove" -> "removed"
Grammar: "in future version" -> "in a future version"

// eslint-disable-next-line no-console
console.warn(
'Attributes matchers are deprecated and they will be remove in future version of Gutenberg.' +
'Please update your attributes definition https://wordpress.org/gutenberg/handbook/reference/attributes/'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar: There should be a space between these two sentences, either prefixing this string or suffixing the previous line. (or separate arguments to console.warn which is maybe not the best behavior to rely on)

@@ -156,6 +156,7 @@ class ButtonBlock extends Component {
</span>,
focus && (
<form
key="form-link"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this was a bug fix I wanted to publish separately and accidentally made it to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can extract it to a separate PR but maybe not worth it

@@ -125,6 +125,17 @@ export function registerBlockType( name, settings ) {
...settings,
};

// Resolve deprecated attributes
settings.attributes = mapValues( settings.attributes, ( attribute ) => {
if ( isFunction( attribute.source ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to return functions from the deprecated matcher? Why not just return the object, and skip this step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it won't match the current API.

it will produce: source: { source: 'text' } which is not the correct attribute definition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be implemented as a filter hook, which has the added advantage of being simpler to drop 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea

@youknowriad youknowriad force-pushed the update/block-attributes branch from c75bbc9 to 4ec5a9a Compare November 16, 2017 15:31
return settings;
}

export default function anchor( { addFilter } ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copypasta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants